fix(http): apply CORS headers to all top-level responses - #3148
Closed
CAOShurong wants to merge 1 commit into
Closed
fix(http): apply CORS headers to all top-level responses#3148CAOShurong wants to merge 1 commit into
CAOShurong wants to merge 1 commit into
Conversation
The SetCorsHeaders middleware was only mounted on the MCP route group, so responses from the OAuth protected resource metadata endpoints and any router-level error fell through without Access-Control-* headers. Browser clients then masked the real failure (a 401 auth challenge or 404) behind an opaque "missing CORS headers" error, making the server appear unreachable from web-based MCP clients. Move the middleware to the top-level chi router so every response class — MCP auth challenges (401), OAuth metadata (200), and fall-through errors — carries CORS headers. The server authenticates via bearer tokens rather than cookies, so wildcard origins remain safe at this layer.
Collaborator
|
Thank you for investigating this and for correctly identifying that the CORS middleware needed to move to the top-level router. That core diagnosis and fix direction are sound.\n\nWe are consolidating the complete change in #3147, which includes the same root-router CORS fix plus the missing readonly/insiders/toolset protected-resource metadata variants, terminal 404 handling for unknown metadata paths, additive exposed-header behavior, the full challenge-to-metadata route contract, and the cross-repository hosted rollout dependencies. Closing this PR as superseded so review and release can proceed through one implementation. Thanks again for the useful contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3095.
Problem
#3095 reports that browser-based MCP clients see
"This MCP server doesn't support web access. Missing CORS headers."when connecting to the hosted server — even though a rawOPTIONSpreflight succeeds with the right headers. The reporter's follow-up captures it precisely: the preflight passes, but subsequent non-2xx responses (401from the auth challenge, and errors from unauthenticated metadata lookups) come back without CORS headers, so the browser hides the real error behind an opaque CORS failure.Root cause
In
pkg/http/server.go,middleware.SetCorsHeaderswas mounted only on the MCP route group:/.well-known/oauth-protected-resource*) had no CORS middleware, so every response from the discovery flow browsers must complete before authenticating was unreadable cross-origin.The MCP endpoints themselves were fine — which is why curl-based checks passed while real browser clients failed.
Fix
Move
r.Use(middleware.SetCorsHeaders)from the MCP group to the top-level chi router, so every response class carries CORS headers:WWW-AuthenticateGET /.well-known/oauth-protected-resource→ 200OPTIONSon the metadata routeWildcard origins remain safe at this layer: the server authenticates via bearer tokens, not cookies, so cross-origin requests cannot exploit ambient credentials (unchanged rationale from the original middleware).
Testing
New
TestTopLevelCORSHeadersOnAllResponses+TestTopLevelCORSPreflightOnMetadataRouteinpkg/http/server_cors_test.go, building the same two-group router layout asRunHTTPServer:Access-Control-Allow-Originand exposesWWW-AuthenticateviaAccess-Control-Expose-HeadersAccess-Control-Allow-OriginOPTIONSpreflight on the metadata route short-circuits with 200 instead of falling through to a CORS-less 404/405Gates:
go build ./...clean;go vet ./pkg/http/...clean;go test ./pkg/http/... ./pkg/context/... -count=1all pass; golangci-lint v2.13.1 reports only the 6 pre-existing gosec hits intoken_test.go(identical on clean main).Note for browser-client authors (re #3095)
With this fix the browser can finally see the
401+WWW-Authenticate: Bearer resource_metadata="..."challenge and the RFC 9728 metadata document. Authentication itself is GitHub OAuth outside this repo's scope — clients should implement the standard MCP authorization flow triggered by that challenge.